Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pref controller: prevent false-positive 'dirty' flag #4721

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 10, 2022

Less headaches, finally.
Fixes https://bugs.launchpad.net/mixxx/+bug/1920844
Current behaviour is annoying if you use a mapping script init() to initialize deck / mixer / effect controls because those controls get re-initialized (= reset) when clicking Okay in the Preferences,.. which was kicking me 'off my horse' during live sets more than once.

See first commit for details.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left some non blocking suggestions.
Give a hint when I should press merge.

@@ -117,5 +117,6 @@ class DlgPrefController : public DlgPreferencePage {
QSortFilterProxyModel* m_pInputProxyModel;
ControllerOutputMappingTableModel* m_pOutputTableModel;
QSortFilterProxyModel* m_pOutputProxyModel;
bool m_bGuiInitialized;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many other preferences pages call slotUpdate() during the constructor which actually initializes the GUI. Here we wait for the first slotUpdate signal, which happens later, I guess. Did you investigate the reason? If yes a comment would be helpful.

The different behaviours is probably one cause that this issue has happened.
But, this is a good band aid fix anyway. Thank you.

For me it would be more clear to rename the variable to something like.
m_initalUpdate; (without b, we have no convention for a b prefix btw).

Copy link
Member Author

@ronso0 ronso0 Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many other preferences pages call slotUpdate() during the constructor which actually initializes the GUI. Here we wait for the first slotUpdate signal, which happens later, I guess.

I'm afraid that pattern is not consistent and startup time is currently prolonged for no good reasons: unrelated/inadequate actions are triggered in slotUpdate, like writing to config, see #4667 (46b690e).
I'll continue cleaning up the preferences pages one by one.
In general, if slotUpdate is clean (only updates the GUI) it's called every time the Preferences are shown, so it doesn't make sense to me to call it during construction, that would only be repepetive.

I picked m_GuiInitialized so we can use
if (m_GuiInitialized) {..
instead of the negation
if (!m_InitialUpdate) {..

@ronso0
Copy link
Member Author

ronso0 commented Apr 11, 2022

I removed a redundant slotUpdate connection. The slot is already connected when adding a the controller page, see DlgPreferences::addPageWidget
Preferences should now also load a little faster since mappings are enumerated only once (per controller page).

@ronso0
Copy link
Member Author

ronso0 commented Apr 11, 2022

@daschuer Please take another look, it's ready for merge IMO

@daschuer
Copy link
Member

Still Looks and works good. Thank you.

@daschuer daschuer merged commit cd4fdf3 into mixxxdj:2.3 Apr 12, 2022
@ronso0 ronso0 deleted the pref-controller-dirty-fix branch April 12, 2022 21:29
@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2022

Thanks for the quick review!

I'll take care of merging to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants